[BUGFIX beta] Deprecate importing htmlSafe and isHTMLSafe from @ember/string by jamescdavis · Pull Request #19339 · emberjs/ember.js

jamescdavis · 2021-01-17T19:43:31Z

As per RFC #236, this deprecates importing htmlSafe and isHTMLSafe from @ember/string.

The tracking issue for RFC #236 says that these will be deprecated via linting, but the linked issue is only a docs change that doesn't make it clear that importing from @ember/string is deprecated. There has been much confusion around whether this is actually deprecated (especially with regard to the @ember/string type definitions).

I'm hoping this can be backported to 3.24 since the the string prototype extensions were deprecated in that version.

rwjblue · 2021-01-18T03:18:19Z

I'm hoping this can be backported to 3.24 since the the string prototype extensions were deprecated in that version.

We typically don't backport deprecations into release 🤔

rwjblue · 2021-01-18T03:20:20Z

packages/ember/index.js

+Ember.String.htmlSafe = deprecateImportFromString('htmlSafe', htmlSafe);
+Ember.String.isHTMLSafe = deprecateImportFromString('isHTMLSafe', isHTMLSafe);


Instead of returning a function that does the deprecation, can you make these a getter that issues the deprecation upon access (those getters would then return the original values)? There are other examples like that in this file.

That would look something like:

Object.defineProperty(Ember.String, 'htmlSafe', { enumerable: true, configurable: true, get() { // do the deprecation // then return the normal value } });

Should htmlSafe and isHTMLSafe get separate deprecation ids? I just made up ember-string.htmlsafe-ishtmlsafe. Is there a pattern I should be following here?

I also just invented the URL https://deprecations.emberjs.com/v3.x/#toc_ember-string-htmlsafe-ishtmlsafe
I'm not sure if there's a pattern I should follow there as well (I was looking at #19234).

Should htmlSafe and isHTMLSafe get separate deprecation ids? I just made up ember-string.htmlsafe-ishtmlsafe. Is there a pattern I should be following here?

Dealers choice (I think either is fine)😊

I also just invented the URL https://deprecations.emberjs.com/v3.x/#toc_ember-string-htmlsafe-ishtmlsafe

I'm not sure if there's a pattern I should follow there as well (I was looking at #19234).

We usually figure out the urls by making the PR and grabbing them from the running app. Once the deprecation guide PR is merged, we can ensure that the URL is correct, then we can land this.

Deprecation app PR: ember-learn/deprecation-app#758

locks · 2021-01-18T15:39:20Z

We typically don't backport deprecations into release 🤔

3.24 is also an LTS candidate, if that impacts any decision.

jamescdavis · 2021-01-18T17:31:12Z

@rwjblue I had based this on the approach used in #19234, but I like the getter approach better. Will update.

As far as backporting to 3.24, my thinking is this usage has already been deprecated for some time (as of the docs change) there was just no deprecation warning ever added or mention of it on http://deprecations.emberjs.com (which has caused confusion, to the point where someone has even removed the usage from the type defs). It's only come to a head recently because of ember-data's new dependency on the @ember/string package and its types overriding DT's, thus causing this usage to no longer typecheck (which I've fixed) making people believe the usage was removed. As @locks mentioned, being an LTS candidate, IMHO we should get a definitive answer as to the state of this usage in that release.

jamescdavis · 2021-01-18T17:39:04Z

As an another example of the confusion, here's a discussion from over a year ago: DefinitelyTyped/DefinitelyTyped#38571 (comment)

rwjblue · 2021-01-18T18:38:03Z

I'm all for providing a definitive answer, ensuring the docs have been updated and the deprecation guide has landed does that sufficiently.

Introducing deprecations in a patch release is just very bad form. For example, a decent number of apps and addon use "throw on deprecation" and releasing a new deprecation warning will start failing their CI. We have a train model for a reason, landing this now and pulling it into the current beta channel should be perfectly fine (especially RE: sending a clear signal).

jamescdavis · 2021-01-18T19:05:01Z

@rwjblue that makes perfect sense! Salsify is one of those apps! 😉
I'll update it to be since 3.25.

jamescdavis · 2021-01-18T19:30:05Z

packages/ember/tests/reexports_test.js

+          'Ember.String.htmlSafe is exported correctly'
+        );
+      }, /Importing htmlSafe from '@ember\/string' is deprecated/);
+      assert.notEqual(glimmer.htmlSafe, undefined, 'Ember.String.htmlSafe is not `undefined`');


These asserts are based on

ember.js/packages/internal-test-helpers/lib/confirm-export.js

Lines 17 to 44 in 26f4aff

export default function confirmExport(Ember, assert, path, moduleId, exportName) {

try {

let desc = getDescriptor(Ember, path);

assert.ok(desc, `the ${path} property exists on the Ember global`);

if (typeof exportName === 'string') {

let mod = require(moduleId);

assert.equal(desc.value, mod[exportName], `Ember.${path} is exported correctly`);

assert.notEqual(mod[exportName], undefined, `Ember.${path} is not \`undefined\``);

} else if ('value' in desc) {

assert.equal(desc.value, exportName.value, `Ember.${path} is exported correctly`);

} else {

let mod = require(moduleId);

assert.equal(desc.get, mod[exportName.get], `Ember.${path} getter is exported correctly`);

assert.notEqual(desc.get, undefined, `Ember.${path} getter is not undefined`);

if (exportName.set) {

assert.equal(desc.set, mod[exportName.set], `Ember.${path} setter is exported correctly`);

assert.notEqual(desc.set, undefined, `Ember.${path} setter is not undefined`);

}

}

} catch (error) {

assert.pushResult({

result: false,

message: `An error occurred while testing ${path} is exported from ${moduleId}.`,

source: error,

});

}

but had to be a little different because it's now a getter in the "reexport" (not strictly a reexport now, I guess) and a value in the original mod.

rwjblue · 2021-01-18T21:51:20Z

packages/ember/index.js

+    since: {
+      available: '3.25',
+    },


Can you set enabled instead of available?

Suggested change

since: {

available: '3.25',

},

since: {

enabled: '3.25',

},

Until the deprecation stages RFC is fully rolled out, we are just using enabled for deprecations (it means that folks get the deprecation right away and don't have to opt in).

rwjblue · 2021-01-18T21:53:11Z

packages/ember/index.js

+      available: '3.25',
+    },
+    until: '4.0.0',
+    url: 'https://deprecations.emberjs.com/v3.x/#toc_ember-string-htmlsafe-ishtmlsafe',


Now that ember-learn/deprecation-app#758 is merged, can you double check the deployed URL to make sure this is right?

Looks good! Things seem to jump around after page load so you don't end up exactly in the right spot, but the anchor but the link does match what's in the TOC. This jumping issue seems to affect all the anchors.

I think the jumping around is the code blocks rendering. I'll make an issue!

rwjblue · 2021-01-19T01:31:47Z

Thank you @jamescdavis!

jamescdavis mentioned this pull request Jan 17, 2021

[@ember/string] augment @ember/string with htmlSafe and isHTMLSafe DefinitelyTyped/DefinitelyTyped#50653

Merged

8 tasks

locks requested a review from rwjblue January 17, 2021 20:25

rwjblue requested changes Jan 18, 2021

View reviewed changes

jamescdavis mentioned this pull request Jan 18, 2021

Deprecate importing htmlSafe and isHTMLSafe from @ember/string ember-learn/deprecation-app#758

Merged

jamescdavis force-pushed the deprecate_htmlSafe_from_ember_string branch from df9ce14 to feab7f3 Compare January 18, 2021 19:27

jamescdavis commented Jan 18, 2021

View reviewed changes

jamescdavis requested a review from rwjblue January 18, 2021 19:42

rwjblue reviewed Jan 18, 2021

View reviewed changes

jamescdavis added 2 commits January 18, 2021 17:20

deprecate htmlSafe/isHTMLSafe reexports on Ember.String

cd60373

update tests for htmlSafe/isHTMLSafe Ember.String deprecation

d9ec17a

jamescdavis force-pushed the deprecate_htmlSafe_from_ember_string branch from feab7f3 to d9ec17a Compare January 18, 2021 22:21

jamescdavis requested a review from rwjblue January 18, 2021 22:25

rwjblue approved these changes Jan 19, 2021

View reviewed changes

rwjblue changed the title ~~[RFC #236] Deprecate importing htmlSafe and isHTMLSafe from @ember/string~~ [BUGFIX beta] Deprecate importing htmlSafe and isHTMLSafe from @ember/string Jan 19, 2021

rwjblue merged commit 82f6c10 into emberjs:master Jan 19, 2021

jamescdavis deleted the deprecate_htmlSafe_from_ember_string branch January 19, 2021 14:14

jamescdavis mentioned this pull request Jan 23, 2021

RFC #0236 - Tracking for Ember.String deprecation emberjs/rfc-tracking#26

Closed

17 tasks

locks mentioned this pull request Feb 19, 2021

Ember 3.25 Release ember-learn/ember-blog#914

Merged

jamescdavis mentioned this pull request Feb 24, 2021

[Bug] Incorrect "import htmlSafe from '@ember/string'" deprecation warning when importing from '@ember/template' #19393

Closed

dependabot bot mentioned this pull request Mar 8, 2021

Bump ember-source from 3.21.3 to 3.25.3 RedisLabs/ember-tagify#23

Closed

dependabot bot mentioned this pull request Mar 16, 2021

chore(deps-dev): bump ember-source from 3.22.1 to 3.25.3 alexlafroscia/ember-esbuild#10

Merged

		Ember.String.htmlSafe = deprecateImportFromString('htmlSafe', htmlSafe);
		Ember.String.isHTMLSafe = deprecateImportFromString('isHTMLSafe', isHTMLSafe);

	export default function confirmExport(Ember, assert, path, moduleId, exportName) {
	try {
	let desc = getDescriptor(Ember, path);
	assert.ok(desc, `the ${path} property exists on the Ember global`);

	if (typeof exportName === 'string') {
	let mod = require(moduleId);
	assert.equal(desc.value, mod[exportName], `Ember.${path} is exported correctly`);
	assert.notEqual(mod[exportName], undefined, `Ember.${path} is not \`undefined\``);
	} else if ('value' in desc) {
	assert.equal(desc.value, exportName.value, `Ember.${path} is exported correctly`);
	} else {
	let mod = require(moduleId);
	assert.equal(desc.get, mod[exportName.get], `Ember.${path} getter is exported correctly`);
	assert.notEqual(desc.get, undefined, `Ember.${path} getter is not undefined`);

	if (exportName.set) {
	assert.equal(desc.set, mod[exportName.set], `Ember.${path} setter is exported correctly`);
	assert.notEqual(desc.set, undefined, `Ember.${path} setter is not undefined`);
	}
	}
	} catch (error) {
	assert.pushResult({
	result: false,
	message: `An error occurred while testing ${path} is exported from ${moduleId}.`,
	source: error,
	});
	}

Uh oh!

Conversation

jamescdavis commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Jan 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

locks commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescdavis commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescdavis commented Jan 18, 2021

Uh oh!

rwjblue commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescdavis commented Jan 18, 2021

Uh oh!

jamescdavis Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Jan 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamescdavis commented Jan 17, 2021 •

edited

Loading

locks commented Jan 18, 2021 •

edited

Loading

jamescdavis commented Jan 18, 2021 •

edited

Loading

rwjblue commented Jan 18, 2021 •

edited

Loading

jamescdavis Jan 18, 2021 •

edited

Loading