use esm for requiring default and deprecated features #18218
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See emberjs/data#6231 (comment) for the motivation.
ghost
commented
Jul 24, 2019
| ts.ScriptTarget.ES2017, | ||
| /*setParentNodes */ true | ||
| const { default: features } = requireEsm( | ||
| '../packages/@ember/canary-features/default-features.js' |
There was a problem hiding this comment.
I could not use @ember/canary-features/default-features.js for the path. Would this be because ember.js' internal package structure differs from ember data?
There was a problem hiding this comment.
I think the local require (e.g. starting with ../packages/) is totally correct here.
ghost
commented
Jul 24, 2019
ghost
commented
Jul 24, 2019
| @@ -1,35 +1,12 @@ | |||
| 'use strict'; | |||
|
|
|||
| const fs = require('fs'); | |||
| const ts = require('./typescript'); | |||
There was a problem hiding this comment.
I see the deprecated features also relies upon this module; I can remove it and merge the deprecated features functionality with this new approach once I sort how the typescript particulars below.
|
Thanks for working on this @efx! |
rwjblue
reviewed
Jul 25, 2019
| ts.ScriptTarget.ES2017, | ||
| /*setParentNodes */ true | ||
| const { default: features } = requireEsm( | ||
| '../packages/@ember/canary-features/default-features.js' |
There was a problem hiding this comment.
I think the local require (e.g. starting with ../packages/) is totally correct here.
ghost
commented
Jul 25, 2019
rwjblue
reviewed
Jul 25, 2019
rwjblue
reviewed
Jul 25, 2019
Co-Authored-By: Robert Jackson <[email protected]>
Co-Authored-By: Robert Jackson <[email protected]>
|
|
Add newly deprecated feature from #18214
ghost
mentioned this pull request
ghost
changed the title
ghost
changed the title
rwjblue
reviewed
Aug 1, 2019
Currently some linting failuresAnd some legit errors in the test run, e.g.: |
|
Hmm, I thought my reexport was working but clearly it does not. Not sure about the exact syntax to export those const declarations from the |
*I know not typescript* This makes it work but now there are 2 copies of these flags in `index.js` and `index.ts`. I am pushing up to avoid blocking and we can consider a follow up PR or any intervening TS guidance to solve the duplication.
|
Closing this as I've run out of steam. I might come back to it once I actually learn TS. May need to spike and try |
ghost
closed this
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See emberjs/data#6231 (comment) for
the motivation.
yarn testblows up locally, which might be related to not properly typing the import ofdefault-features.jsor not correctly exporting it in@ember/canary-features.@rwjblue if you have free moments to give me pointers / review here that would be great.